Repository navigation
Track runtime shutdown and retain foreign cleanup - #64
ChrisKrawczyk wants to merge 7 commits into
Conversation
Add fallible preparation and startup, immutable shutdown deadlines, and parent-owned execution cleanup with truthful quiescence outcomes. Retain foreign invocation cleanup, preserve durable replay decisions, and add lifecycle diagnostics, controlled regression coverage, and docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 73748d4f-2eb6-47d2-9dbc-af8681e80e83
Merge main's shutdown and replay improvements. Replace the custom task tree and total-deadline machinery with upstream tracking plus narrow activation, fault-reporting and foreign-retirement capabilities. Preserve native grace semantics, diagnostics, recovery coverage and checksum-verified replay fixtures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 73748d4f-2eb6-47d2-9dbc-af8681e80e83
|
Upstream-first reconciliation is pushed in 82b2572 (normal merge of main, no history rewrite). GitHub reports this draft conflict-free. Native ownership now uses upstream TaskTracker, direct dispatcher handles and child abort guards; the custom task-group and core total-deadline modules are removed. Narrow fallible activation, typed actual-cleanup results and foreign finalization/retirement remain. Legacy native grace-then-cleanup and provider-cancellation semantics are preserved; SDKs own their bounded observations. The feature-only diff falls from 6,976 to 4,774 changed lines, about 32%, excluding imported upstream fixes. The independent scoped integration review is PASS after correcting an inherited .NET concurrent-test assertion. Current core verification passed 156 focused cases (one recorder ignored); doctests passed 46 with 74 ignored. The reviewed tree exactly matches this commit. Coordinated adapters: microsoft/duroxide-net#21, microsoft/duroxide-node#23, microsoft/duroxide-python#25. These remain draft/development integrations: published core 0.1.32 lacks the additive APIs, so approved registry dependency delivery, PostgreSQL, non-Windows/five-RID, broader regression and hosted CI gates remain OPEN. Nothing was merged, released or published. |
Address the ten accepted core review findings, preserving panic compatibility and upstream lifecycle ownership. Add bounded-cardinality metrics and focused production/instrumented regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 73748d4f-2eb6-47d2-9dbc-af8681e80e83
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 73748d4f-2eb6-47d2-9dbc-af8681e80e83
Thom Chiovoloni (thomcc-work)
left a comment
There was a problem hiding this comment.
Mostly good. A few issues found with the help of AI.
Also, this changes the cancellation behavior for native (Rust) a good amount, since now a panic dtor stops the whole runtime. (Admittedly, panicking in a dtor isn't great, but we do it some). Also, it adds a good amount of overhead to native use (much more locking and such). Maybe that's fine? Anyway, I'd like to take another look at it after the changes.
| category = "invocation_destruction_failed", | ||
| "Orchestration driver destruction failed; invocation cleanup remains owned" | ||
| ); | ||
| state.lock().unwrap().failures.extend(failures); |
There was a problem hiding this comment.
Hmm, runtime-wide fault reporting will wait until the invocation retires, but before that happens, execution can await a provider read needed to resolve a legacy child’s parent execution. Report the fault before any subsequent await, while preserving the valid decision and cleanup ownership.
There was a problem hiding this comment.
🐾 Implementation Reviewer 🤖:
Addressed in b3d6aa9. The normal replay handoff now transfers recorded turn failures and reports them before any later provider await. It moves only the failure vector: finalization state, attachment membership, the captured decision and independently owned retirement remain intact. The redundant unprotected destruction diagnostic is removed; fault state is recorded before protected diagnostics run.
The regression holds the legacy parent-provider read and verifies that the turn-originated fault is already visible, while registered foreign cleanup remains held. Stamped-parent, no-fault, repeated-take and panic-once diagnostic controls cover routing, once-only reporting and retained cleanup. Force/acknowledgement gates verify unchanged committed history.
The documentation also distinguishes preserving a decision from promising its eventual commit: earlier zero-grace force can cancel an acknowledgement that has not committed. It cannot undo committed history.
|
|
||
| let work_handles = runtime.clone().start_work_dispatcher(activity_registry); | ||
| runtime.tasks.lock().await.extend(work_handles); | ||
| fn request_rollback(self: &Arc<Self>) { |
There was a problem hiding this comment.
The runtime can stop every dispatcher while its containing process remains healthy. E.g. hosts get ERROR logs, but no dedicated stop/fault notification or health state to drive replacement or process exit. This would be a problem for native services and PGRX workers.
Ideally, we'd have either a passive signal/status and document the supervision pattern, or we'd explicitly establish log-based supervision as the way to do things.
There was a problem hiding this comment.
🐾 Implementation Reviewer 🤖:
Addressed in b3d6aa9 with Runtime::stop_status() and an owned RuntimeStopObserver from stop_observer(). The observer exposes state() and cancellable changed(last_seen) observation of retained NotRequested / Requested / Faulted state. Faults are sticky, including escalation after rollback was already requested.
The observer holds only a private status receiver, not the Runtime, provider, registries or callbacks. Late and independent readers retain visibility; cancellation does not request shutdown or change cleanup ownership. An unseen final state is delivered before source closure. Neither status nor closure proves cleanup. Tests cover publication ordering, zero/late readers, cancellation, collection, escalation and stop/fault races.
The native lifecycle guide includes a supervision example and a stop-versus-cleanup diagram. Hosts must keep the executor progressing and still drive shutdown to its typed cleanup result. Passive SDK exposure and diagnostic-only .NET host monitoring are explicit follow-up work; this commit does not claim to solve those bindings' spontaneous-fault visibility or automatically stop/restart a host.
There was a problem hiding this comment.
🐾 Implementation Reviewer 🤖:
The SDK follow-up described above is now implemented and pushed: microsoft/duroxide-net#21 at ba8d639, microsoft/duroxide-node#23 at ad34d92, and microsoft/duroxide-python#25 at e74ea99. Core is reconciled with main in eae7780.
All three bindings expose owned, metadata-only stop/fault observation. Default .NET hosting monitors the actual selected lifecycle owner's optional capability, including retained/early faults; unsupported custom owners receive a clear warning. Monitoring remains diagnostic-only: no automatic host stop, process exit, or restart. Canceling or closing observation does not stop the Runtime or release its cleanup obligations, and neither status nor source closure proves cleanup.
Fresh qualification against the reconciled core covers fault visibility while a foreign callback remains held, cancellation/closure, and observer collection. The eight SDK cells pass 711 cases with 46 explicit hook-only skips; .NET native builds are release, while Node/Python native builds are dev. A separate real non-test, hook-free core proof passes 10 cases. The full core feature modes pass 1,350/1,347 cases and preserve the early-fault and two-turn replay corrections.
The PR description now covers the delivered SDK work, admission/lifecycle integration, the approved SQLite contention/cancelled-acquisition fixes, and qualification limitations. These are verified branch implementations, not a package-release or automatic-supervision policy claim. Existing re-review remains requested; the threads are left for reviewer resolution.
| if cleanup.finalization_failed() { | ||
| return Err(()); | ||
| } | ||
| if finalized { |
There was a problem hiding this comment.
From AI: The new acceptance test schedules work during finalization and verifies its initial persistence. Extending that pattern to a second turn produces: "nondeterministic: history schedule but no emitted action". Reproduced. Historical matching happens before the current finalizer emits the corresponding action. This is a contract/test gap, not proof that conforming language adapters are broken. Clarify that admitted decisions must be reproduced during normal replay, and test the intended protocol across two turns. Supporting arbitrary finalizer-only scheduling would require additional replay support.
There was a problem hiding this comment.
🐾 Implementation Reviewer 🤖:
Addressed in b3d6aa9 without extending the replay engine. Registration rustdoc and the lifecycle guide now state that finalization admits a batch before the snapshot; normal handler replay must reproduce admitted schedules and detached starts before history matching reaches them. The original acceptance test is explicitly named as first-snapshot coverage.
Four two-turn controls now distinguish the contracts: ordinary replay completes; normal replay of a first-turn finalizer-admitted batch stays Running on its expected event; finalizer-only detached-start and activity patterns fail locally with the exact nondeterminism messages. The negative cases do not fault the Runtime. The controls also check the immutable first-history prefix, correlated activity completion, child identity and independently retained foreign cleanup across both turns.
Report protected turn faults before provider awaits while retaining finalization, valid decisions and foreign cleanup ownership. Add owned stop/fault observers, monotonic publication and lifecycle regressions. Document normal replay and add two-turn finalization controls without extending replay semantics. Ref: #64 (comment) Ref: #64 (comment) Ref: #64 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 73748d4f-2eb6-47d2-9dbc-af8681e80e83
|
🐾 Implementation Reviewer 🤖: Core review corrections are pushed in b3d6aa9. Replies on all three inline threads explain early protected fault reporting, owned native stop observation, and two-turn replay controls. The following is the updated qualification record; the PR description still contains the earlier documentation-pass snapshot. Containment trade-off: this revision retains Runtime-wide stop for faults caught at protected internal/destruction/finalization/registered-cleanup boundaries. Ordinary application errors and handler-poll panics remain operation-local; a destructor within that catch can also fail locally. The catch boundary matters, not merely whether a Bookkeeping cost: the earlier bounded base-to-reviewed-head replay-only comparison measured +0.216 microseconds (+12.8%) for an immediate turn and +0.290 microseconds (+3.13%) for five-activity scheduling, with one additional allocation. That harness bypassed the newly changed execution handoff, so those numbers do not qualify this correction. A new matched comparison from
Five alternating timing pairs used hook-free Windows release builds, 10,000 warmups and 200,000 measured iterations per process. Allocation diagnostics ran separately. Immediate ranges were 4.369-4.398 / 4.153-4.224 us; five-activity ranges were 14.596-14.931 / 14.354-14.734 us and overlap. This finds no additional per-turn allocation or measured regression in these workloads, not a general speedup or zero-overhead claim. It excludes provider I/O, dispatcher contention, actual activity execution, foreign completion and cold Runtime/observer construction. The two harnesses are not interchangeable end-to-end measurements. Final-core qualification: formatting and all-target/all-feature Clippy passed. The focused default-feature and all-feature runs each passed 157 cases; each Rustdoc run passed 46 with 74 existing ignored examples. Fresh .NET, Node and Python native assets were rebuilt against the same sealed core. .NET passed 37 native, 176 instrumented managed and 96 hook-free managed cases. Node passed all 37 instrumented cases and 26 hook-free cases with 11 expected hook-only skips. Python 3.12 and 3.13 each passed 52 instrumented cases and 43 hook-free cases with nine expected hook-only skips; each run includes 13 native-independent probe-support cases. The new guide example compiled and its Mermaid diagram rendered locally. These are Windows x64 local results. SDK native assets used dev profiles; performance used hook-free release. This is compositional core/SDK ownership evidence, not proof that every SDK directly injects the same turn-destructor fault. Compatible registry pins, hosted/platform/release-package qualification, and passive SDK/default-host fault visibility remain separate gates. No merge, release or publication is requested. Review threads remain open for your assessment. |
Label the replay and capability-filtering progress records as dated snapshots and link to current guides. Remove misleading current-status wording while preserving design rationale, recorded decisions and release history. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 73748d4f-2eb6-47d2-9dbc-af8681e80e83
Preserve actual Tokio panic identity and protected admission/handler destruction. Reserve SQLite writers before read-modify-write transactions and roll back cancelled acquisitions before pool reuse. Order duplicate-event and early-event fixtures explicitly without extending deadlines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 73748d4f-2eb6-47d2-9dbc-af8681e80e83
Summary
Make Runtime startup, stop observation, and shutdown explicit.
Use upstream
TaskTracker, dispatcher handles, and abort-on-drop guards.Runtime::prepareand falliblestart_execution. Retain partial-startup rollback until cleanup ends.Drained/Forcedshutdown outcomes. Both successful outcomes prove tracked execution and registered cleanup ended.Runtime::stop_status()and an ownedRuntimeStopObserver. Observation retains status metadata, not the Runtime or its callbacks.The branch now includes main
36b789dthrough merge commiteae7780.Upstream nonblocking activity admission retains its deferral, attempt, routing,
and telemetry behavior. Admission and handler destruction are protected
independently, including forced cancellation; application panic history keeps
the actual Tokio task identity.
Qualification also reproduced an inherited SQLite read-to-write contention
failure on unmodified main. The approved fix reserves writer access with
BEGIN IMMEDIATEfor the five read-before-write transaction paths. Pool returnrolls back transactions left by cancellation between SQLx's custom-BEGIN
acknowledgement and creation of its transaction guard. Rollback errors use
SQLx's existing reporting and connection-discard path. Existing lock timeouts,
attempt accounting, retry classification, and stored formats are unchanged.
Two event fixtures now establish their required ordering instead of racing
dispatch; their behavioral assertions and deadlines remain intact.
flowchart TD Stop["Accept stop: prevent new work"] --> Grace["Allow admitted work to finish during grace"] Grace -->|Work finishes| Cleanup["Wait for tracked native and registered foreign cleanup"] Grace -->|Grace expires| Force["Abort remaining async tasks"] Force --> Cleanup Cleanup --> Result["Return Drained, Forced, or a cleanup error"] Cleanup -.-> Pending["Blocked work can keep cleanup pending indefinitely"]Native grace limits the wait before force, not the total cleanup wait.
Language SDKs retain cleanup and separately limit their callers' waits.
A timeout does not permit callback release.
Independent providers, Clients, and provider token-refresh tasks keep their own lifetimes.
Review focus
Review lifecycle ownership in
src/runtime/mod.rs,invocation_scope.rs, andtask_safety.rs; admission/destruction indispatchers/worker.rs; early faultreporting in
execution.rs; and SQLite transaction acquisition/pool reuse.Protected internal and cleanup faults request zero-grace shutdown of the
affected Runtime. This can interrupt unrelated work and cause retries or
repeated effects. Ordinary application errors and handler-poll/admission panics
remain operation-local. The catch boundary determines fault scope; a destructor
panic is not always runtime-wide. Admission-guard destruction faults are cleanup
faults, not admission rejections.
Stop status is retained and can escalate from
Requestedto stickyFaulted.NotRequestedis not a readiness check.Neither status nor observer closure proves cleanup or permits resource release.
Finalization admits a batch; it is not another replay stage. Normal replay must
reproduce admitted durable operations before history matching reaches them.
A detected pre-snapshot finalization failure rejects the attempt's new batch.
Later faults preserve a valid decision, but earlier force can cancel an
uncommitted acknowledgement. Committed history stays committed. Shutdown adds
no transaction, compensation, or exactly-once guarantee.
The three existing review corrections remain in place: faults precede held
provider reads, observation retains only metadata, and two-turn controls
distinguish normal replay from unsupported finalizer-only scheduling.
The lifecycle guide
documents these contracts with examples and ownership diagrams.
Two older progress records remain clearly labeled historical; their rationale is preserved.
SDK follow-up
Passive observation is now implemented and pushed on the coordinated branches:
microsoft/duroxide-net#21, microsoft/duroxide-node#23, and microsoft/duroxide-python#25.
Default .NET hosting provides diagnostic-only monitoring of the actual selected
lifecycle owner. Unsupported custom owners receive an explicit warning; capable
owners expose the optional observation capability without forcing activation.
Observer cancellation, closure, and source completion remain separate from
execution cleanup. No implementation automatically stops a host, terminates a
process, or restarts a worker. These are branch implementations, not published packages.
Current local qualification
Qualified source bytes match the reconciled commits. Results are Windows x64:
Each core feature mode covers all 61 suites and retains one existing ignored
historical fixture recorder. The unchanged exhaustive cancellation harness
enumerates 587,844 cases, individually asserts 293,922 new-policy cases, and
retains its legacy digest. Only that harness uses test opt-level 2, with debug
assertions and the original 180-second guard; other suites remain unoptimized.
The earlier unoptimized timeout is preserved, not counted as a pass.
A separate proof links a real non-test, hook-free core: seven public lifecycle
and three foreign-retirement metrics cases pass. Cargo's ordinary test builds
are not presented as equivalent hook-free evidence.
The eight SDK cells total 711 passes and 46 explicit hook-only skips.
.NET native DLLs use release builds; Node uses NAPI dev builds; Python uses
dev ABI3 wheels. Actual loaded native paths/hashes and sole local-core/provider
graphs are checked. .NET production compiles out its private-hook cases.
Earlier native assets are not substituted for these builds.
Formatting and all-target/all-feature Clippy pass. Both Rustdoc builds succeed
with seven link warnings in unchanged source files. Each doctest mode passes
46 cases with 74 existing ignored examples.
The October 28 qualification record
contains earlier bounded release-profile performance measurements.
Those are historical, not new measurements of this integration, and do not
establish end-to-end performance or zero overhead.
Remaining gates
Maintainer re-review/approval, merge, and compatible core publication precede
final SDK registry pins and locks. Final registry-only, hosted, release-profile,
supported-platform, and installed-package qualification remain open.
Node's imported provider update does not qualify a shared production PostgreSQL migration.
Current-head hosted CI is a separate gate; these local results do not claim it has passed.
The Node run at the reconciled head
currently fails to compile against registry
duroxide0.1.32: that publishedsource lacks the unreleased lifecycle/observer APIs, including
RuntimeStopObserver,RuntimeStopStatus, andRuntime::prepare.This is the compatible-core-publication gate above, not evidence that the
local-source integration has registry or platform qualification.
No release version, tag, package publication, or publishing dispatch is requested.
Local workflow artifacts and binaries are excluded.
Generated with PAW.